Skip to content

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Oct 11, 2025

User description

🔗 Related Issues

Fixes #15329

  • New feature (non-breaking change which adds functionality and tests!)

New API surface:

var permissions = bidi.AsPermissions();
await permissions.SetPermissionAsync("geolocation", PermissionState.Denied, newPage, userContext.UserContext);

PR Type

Enhancement


Description

  • Enable external BiDi module creation and extension

  • Refactor module architecture with InternalModule base class

  • Expose Broker and JsonOptions for external access

  • Simplify module creation with public static method


Diagram Walkthrough

flowchart LR
  BiDi["BiDi Class"] -- "exposes" --> Broker["Internal Broker"]
  BiDi -- "exposes" --> JsonOptions["JSON Options"]
  Module["Base Module"] -- "extends" --> InternalModule["InternalModule"]
  InternalModule -- "used by" --> AllModules["All Built-in Modules"]
  Module -- "public creation" --> ExternalModules["External Modules"]
Loading

File Walkthrough

Relevant files
Enhancement
13 files
BiDi.cs
Expose internal properties and refactor module creation   
+26/-21 
InternalModule.cs
Add new base class for internal modules                                   
+34/-0   
Module.cs
Refactor to support external module creation                         
+6/-7     
BrowserModule.cs
Change inheritance to InternalModule                                         
+1/-1     
BrowsingContextModule.cs
Change inheritance to InternalModule                                         
+1/-1     
EmulationModule.cs
Change inheritance to InternalModule                                         
+1/-1     
InputModule.cs
Change inheritance to InternalModule                                         
+1/-1     
LogModule.cs
Change inheritance to InternalModule                                         
+1/-1     
NetworkModule.cs
Change inheritance to InternalModule                                         
+1/-1     
ScriptModule.cs
Change inheritance to InternalModule                                         
+1/-1     
SessionModule.cs
Change inheritance to InternalModule                                         
+1/-1     
StorageModule.cs
Change inheritance to InternalModule                                         
+1/-1     
WebExtensionModule.cs
Change inheritance to InternalModule                                         
+1/-1     

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 11, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #15329
🟢 Enable a minimal, public mechanism in .NET to implement extendable third-party BiDi
modules without relying on internal members.
Expose necessary BiDi primitives (e.g., broker/transport and JSON serialization
options/context) for external modules to function.
Provide a straightforward way for consumers to create/use external BiDi modules (e.g., a
factory/extension pattern).
Maintain compatibility with existing built-in modules while refactoring to allow
extensibility.
Include example usage feasibility similar to creating a Permissions module as an external
extension.
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 11, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Validate serializer context against options

In the Create method, validate that the provided cachedContext is compatible
with the jsonOptions by comparing their Options properties, and throw an
ArgumentException if they do not match.

dotnet/src/webdriver/BiDi/Module.cs [34-45]

 public static TModule Create<TModule>(BiDi bidi, JsonSerializerOptions jsonOptions, JsonSerializerContext? cachedContext = null)
     where TModule : Module, new()
 {
+    if (cachedContext is not null && cachedContext.Options != jsonOptions)
+    {
+        throw new ArgumentException("The provided JsonSerializerContext is not compatible with the provided JsonSerializerOptions.", nameof(cachedContext));
+    }
+
     TModule module = new()
     {
         Broker = bidi.Broker,
     };
 
     module.JsonContext = cachedContext ?? module.Initialize(jsonOptions);
 
     return module;
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a potential configuration issue in the public Create method and proposes a validation check to prevent runtime errors, which enhances the robustness of the API.

Low
Learned
best practice
Validate constructor input URL

Add argument validation to ensure url is not null/empty and is an absolute URI,
throwing ArgumentException/ArgumentNullException with clear messages.

dotnet/src/webdriver/BiDi/BiDi.cs [64-83]

 private BiDi(string url)
 {
-    var uri = new Uri(url);
+    if (url is null) throw new ArgumentNullException(nameof(url));
+    if (string.IsNullOrWhiteSpace(url)) throw new ArgumentException("URL must not be empty.", nameof(url));
+    if (!Uri.TryCreate(url, UriKind.Absolute, out var uri))
+        throw new ArgumentException("URL must be a valid absolute URI.", nameof(url));
 
     JsonOptions = DefaultBiDiOptions();
 
     _jsonContext = new BiDiJsonSerializerContext(JsonOptions);
 
     Broker = new Broker(this, uri, JsonOptions);
     SessionModule = Module.Create<Session.SessionModule>(this, JsonOptions, _jsonContext);
     BrowsingContext = Module.Create<BrowsingContext.BrowsingContextModule>(this, JsonOptions, _jsonContext);
     Browser = Module.Create<Browser.BrowserModule>(this, JsonOptions, _jsonContext);
     Network = Module.Create<Network.NetworkModule>(this, JsonOptions, _jsonContext);
     InputModule = Module.Create<Input.InputModule>(this, JsonOptions, _jsonContext);
     Script = Module.Create<Script.ScriptModule>(this, JsonOptions, _jsonContext);
     Log = Module.Create<Log.LogModule>(this, JsonOptions, _jsonContext);
     Storage = Module.Create<Storage.StorageModule>(this, JsonOptions, _jsonContext);
     WebExtension = Module.Create<WebExtension.WebExtensionModule>(this, JsonOptions, _jsonContext);
     Emulation = Module.Create<Emulation.EmulationModule>(this, JsonOptions, _jsonContext);
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate inputs early with precise checks to prevent null/invalid argument issues.

Low
  • Update

Copy link
Member

@nvborisenko nvborisenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No value,. In any case thank you.

@RenderMichael RenderMichael changed the title [dotnet] Enable external bidi modules [dotnet] Implement third-party Permissions module Oct 11, 2025
return bidi;
}

public static PermissionsModule AsPermissions(this BiDi bidi)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely should be in another class. The reason is we are CLS compatible (I don't still understand why, but anyway), meaning some languages cannot deal with extension methods. They will use static method: WebDriverExtensions.AsPermissions(bidi). But it is not extension of WebDriver.

We should introduce good name for static class, and its namespace.

BiDiExtensions.AsPermissions(bidi)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When Bluetooth module comes to under umbrella, then what? Should we have single static class for all them, or they define own static class in theirs namespace?

I vote for "everything in their namespace". It would be good pattern to demonstrate good practices how to develop module. Separate namespace is not a problem, IDE is still able to suggest extension method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extension methods do not affect CLS compliance. Users can call WebDriverExtensions.AsPermissions(bidi) even on extension methods.

But I agree that extensions should be in separate files. I will make those changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern is about naming: WebDriverExtensions.AsPermissions(bidi) - it is not extension for WebDriver, it is for BiDi.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I followed the two suggestions: WebDriverExtensions -> BiDiExtensions, and permissions in its own area

Copy link
Member

@nvborisenko nvborisenko Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we add all extension methods in one BiDiExtensions class under root BiDi namespace. And a developer wants to create his own extension under BiDi namespace. Is it criminal? Does he break end-users? I am thinking over naming conflicts. (UPD: I mean class with the same FQN but in different assemblies)

Personally I vote for # 2, but let's analyze impact. # 1 is very straightforward, even if end-user imports namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some libraries that wrap/extend Selenium that use the OpenQA.Selenium namespace segment, so it’s not a marginal concern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My final confidence: # 1 - 65%, # 2 - 35%;

Please think and share your confidence (don't cheat).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me: # 1 - 75%, # 2 - 25%

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YevgeniyShunevych please just vote blindly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[🚀 Feature]: [dotnet] Provide a possibility to implement BiDi Permissions module

4 participants